Add null check for connection to prevent AttributeError on drain_events#457
Conversation
|
@ChickenBenny please review this |
There was a problem hiding this comment.
Pull request overview
This PR addresses a shutdown-time crash where AbstractChannel.wait() can attempt to call drain_events() on a None connection, leading to an AttributeError during worker shutdown / channel revive logic.
Changes:
- Add a local
conn = self.connectionreference insidewait()’s loop. - Raise
RecoverableConnectionErrorwhenself.connectionisNoneinstead of callingdrain_events().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
auvipy
left a comment
There was a problem hiding this comment.
can you please add proper unit tests and possibly Integration tests for this proposed change?
|
Hi @Cyanty, thanks for looking into this and providing a quick fix! The local variable assignment However, I have a couple of thoughts on this approach: 1. Is this treating the symptom rather than the root cause? Looking at the traceback in #456, here is a step-by-step breakdown:
The root cause appears to be in def _on_close(self, reply_code, reply_text, class_id, method_id):
self.send_method(spec.Channel.CloseOk)
- if not self.connection.is_closing:
+ if not self.connection.is_closing and not self.is_closing:
self._do_revive()
raise error_for_code(
reply_code, reply_text, (class_id, method_id), ChannelError,
)That said, the null check in this PR is still a worthwhile defensive guard regardless of whether the root cause is also addressed — a belt-and-suspenders approach. Would it be worth opening a separate issue or follow-up PR for the 2. Missing test coverage We absolutely need a unit test for this new exception path. Please add a test case in |
|
I have submitted a new PR #458 to fix the root cause of this issue. It adds the I think this PR and #457 complement each other nicely—one fixes the root cause, and the other acts as a defensive fallback. |
|
I think we can accept both? |
|
Thank you for the prompt response from the community. ChickenBenny's suggestion is even better and makes the code more robust. I really appreciate your reply and explanation. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
LGTM~ |
While null checks exist elsewhere, this specific call was overlooked. This aligns with existing error handling patterns in the codebase.
AttributeError: 'NoneType' object has no attribute 'drain_events'during worker shutdown (Channel attempts revival during close) #456